Skip to content

Conversation

@ocofaigh
Copy link
Contributor

@ocofaigh ocofaigh commented Dec 10, 2024

Description

(NOTE: upgrade test had to be skipped because the main branch code fail due to this bug)

Root module:

  • removed the input existing_kms_instance_guid and updated the code to parse the GUID from the KMS key CRN
  • updated several variable descriptions to make them more clear for consumers
  • updated the variable validation for backup_encryption_key_crn as the regex in main branch would not of accepted eu-es for example, which is supported for HPCS. I no longer check for region in the regex so we don't have to maintain it when new regions support is added in the future.
  • added new boolean use_same_kms_key_for_backups (which will fix the problem that we saw in https://github.ibm.com/GoldenEye/issues/issues/11876)
  • renamed kms_encryption_enabled to use_ibm_owned_encryption_key so we are consistent with the fscloud and DA.
  • updated some of the regex validation in main.tf (eventually we will bump to tf 1.9 and use tf cross variable validation but want to do that in a separate PR)
  • updated parsing logic to now use our crn-parser submodule

FSCloud updates:

  • exposed the use_default_backup_encryption_key input
  • added validation to the backup_encryption_key_crn input
  • exposed the use_same_kms_key_for_backups input
  • added validation to the kms_key_crn input
  • removed the local variable validation regex checks (they will be covered by the root level module)

DA updates:

  • added missing variables to the ibm_catalog.json
  • updated several variable descriptions to make them more clear for consumers
  • exposed the input use_default_backup_encryption_key
  • removed existing_backup_kms_instance_crn and removed support for creating a backup key. It gets too complicated then as we would need to expose the ability to use existing key rings etc. If user really wants to use a different key for backups (which seems to be an edge use case), they can use existing_backup_kms_key_crn and use an existing key.
  • skip_iam_authorization_policy has been renamed skip_es_kms_auth_policy since the DA also supports creating secrets manager auth policy
  • scope the cross account backup key auth policy the exact key to be consistent with all other KMS auth policies created by the DA / module.
  • updated parsing logic to now use our crn-parser submodule

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@ocofaigh
Copy link
Contributor Author

/run pipeline

@ocofaigh ocofaigh marked this pull request as draft December 10, 2024 18:16
@ocofaigh
Copy link
Contributor Author

/run pipeline

@ocofaigh
Copy link
Contributor Author

/run pipeline

@ocofaigh
Copy link
Contributor Author

/run pipeline

@ocofaigh ocofaigh marked this pull request as ready for review December 11, 2024 11:07
@jor2
Copy link
Member

jor2 commented Dec 11, 2024

Looks good. Now lets try use the same approach in other ICD modules.

@ocofaigh ocofaigh merged commit 15af9b4 into main Dec 11, 2024
2 checks passed
@ocofaigh ocofaigh deleted the kms-refactor branch December 11, 2024 17:19
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants